chore: CI and code-quality improvements#121
Conversation
Regression:
|
| ROM | Status | Details | Diff |
|---|---|---|---|
| jagniccc | ✅ PASS | 0 pixels differ | - |
| yarc | ✅ PASS | 0 pixels differ | - |
| jagniccc (determinism) | ✅ PASS | identical across runs | - |
| yarc (determinism) | ✅ PASS | identical across runs | - |
| jagniccc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| yarc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| jagniccc (save state) | ✅ PASS | round-trip matches | - |
| yarc (save state) | ✅ PASS | round-trip matches | - |
| jagniccc (rewind) | ✅ PASS | rewind matches | - |
| yarc (rewind) | ✅ PASS | rewind matches | - |
Platform: Darwin arm64
Updated by CI at 2026-05-01T06:59:42.270Z
Regression:
|
| ROM | Status | Details | Diff |
|---|---|---|---|
| jagniccc | ✅ PASS | 0 pixels differ | - |
| yarc | ✅ PASS | 0 pixels differ | - |
| jagniccc (determinism) | ✅ PASS | identical across runs | - |
| yarc (determinism) | ✅ PASS | identical across runs | - |
| jagniccc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| yarc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| jagniccc (save state) | ✅ PASS | round-trip matches | - |
| yarc (save state) | ✅ PASS | round-trip matches | - |
| jagniccc (rewind) | ✅ PASS | rewind matches | - |
| yarc (rewind) | ✅ PASS | rewind matches | - |
Platform: Linux x86_64
Updated by CI at 2026-05-01T06:59:47.159Z
Regression:
|
| ROM | Status | Details | Diff |
|---|---|---|---|
| jagniccc | ✅ PASS | 0 pixels differ | - |
| yarc | ✅ PASS | 0 pixels differ | - |
| jagniccc (determinism) | ✅ PASS | identical across runs | - |
| yarc (determinism) | ✅ PASS | identical across runs | - |
| jagniccc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| yarc (frameskip) | ✅ PASS | skip=0 matches skip=3 | - |
| jagniccc (save state) | ✅ PASS | round-trip matches | - |
| yarc (save state) | ✅ PASS | round-trip matches | - |
| jagniccc (rewind) | ✅ PASS | rewind matches | - |
| yarc (rewind) | ✅ PASS | rewind matches | - |
Platform: Linux aarch64
Updated by CI at 2026-05-01T06:59:51.161Z
There was a problem hiding this comment.
Pull request overview
Updates the repo’s AI-assistance contributor guide (CLAUDE.md) as part of ongoing CI/developer-experience cleanup, focusing on removing stale references and tightening guidance without changing source code.
Changes:
- Condenses and restructures
CLAUDE.md(project/build/C89/testing guidance). - Removes references to non-existent docs and updates CI/tooling references.
- Adds guidance about large generated sources (notably
src/m68000/cpuemu.c).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The claude-code-review.yml workflow was failing on every PR (run was exiting non-zero after the action's bun-install step). It also duplicated review functionality already provided by other tooling. The on-demand @claude bot in claude.yml is left intact since it only runs when explicitly mentioned and is not failing. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Adds .editorconfig pinning UTF-8, LF line endings, final newlines, and
trailing-whitespace stripping repo-wide. YAML gets 2-space indent;
shell scripts get space-only (size left unenforced for existing 3-space
files). Vendored / machine-generated trees are exempt:
src/m68000 (UAE 68K core), src/bios/jag*.c (bin2c hex tables), and
libretro-common (vendored).
Wires editorconfig-checker v3.0.3 into c-cpp.yml as a new job so
deviations show up at PR time. .ecrc carries the binary-file glob
exclusions (ROMs, baselines, build artifacts, vendored sources) since
.editorconfig itself can't skip files entirely.
Drive-by cleanups to satisfy the new check:
- Strip trailing whitespace from CODEOWNERS, Makefile, src/cd/cdrom.c,
src/jerry/jerry.h, src/tom/{blitter,gpu,op,tom}.c.
- Convert .gitlab-ci.yml from CRLF to LF.
Co-Authored-By: Claude Opus 4.7 <[email protected]>
Resolves the "Node.js 20 actions are deprecated" warnings emitted on every workflow run (June 2 2026 deadline). actions/checkout v4 -> v5 (Node 24) actions/upload-artifact v4 -> v5 actions/download-artifact v4 -> v5 actions/cache v4 -> v5 actions/github-script v7 -> v8 mymindstorm/setup-emsdk v14 -> v16 Conservative N+1 major bumps where possible. Latest tags are further ahead (checkout v6, upload-artifact v7, etc.) but introduce additional behavioral changes; v5 is the stable Node-24 transition release. msys2/setup-msys2@v2, nttld/setup-ndk@v1, and ilammy/msvc-dev-cmd@v1 are already at their current major versions. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Replaces the split GIT_VERSION/CORE_VERSION wiring (Makefile -DGIT_VERSION
+ libretro.c hardcoded "v2.2.0") with a generated src/core/version.h that
both sites read.
CORE_BASE_VERSION (Makefile) single source of truth
v
scripts/gen-version-h.sh portable composer
v
src/core/version.h (gitignored) #define CORE_VERSION ...
v
libretro.c #include "version.h"
Generation is content-aware (cmp + mv), so unchanged content leaves
mtime untouched and incremental builds stay incremental. Only changing
the version (or the git short rev) invalidates libretro.o.
CI: msvc-check compiles libretro.c directly without going through make,
so it runs scripts/gen-version-h.sh as a pre-step.
Also fixes version-bump.yml which was greping `library_version = "v..."`
in libretro.c -- a literal that no longer exists since the assignment
became `= CORE_VERSION`. The workflow now reads/writes the Makefile's
CORE_BASE_VERSION line.
Co-Authored-By: Claude Opus 4.7 <[email protected]>
Mach-O ld64 ignores --version-script (so link.T did nothing on Apple
platforms) but it does honour -exported_symbols_list. Adds:
exports.list production retro_* only (mirrors link.T)
exports-test.list wide symbol set for white-box test harnesses
(mirrors link-test.T)
Threaded into the osx, ios, and tvos-arm64 platform stanzas via a new
$(MACHO_EXPORTS_FLAGS) variable that toggles between the two lists
based on TEST_EXPORTS=1, mirroring the existing GNU-ld choice.
Effect on macOS arm64 production .dylib:
before: every internal DSP/GPU/blitter/68K symbol exported (~hundreds)
after: 46 retro_* entry points, zero leaks
CI: c-cpp.yml gets a "Verify Mach-O symbol gating" step that runs
nm -gU on the production .dylib and fails if anything outside retro_*
is exported. Runs before the test step (which would relink with the
wide list).
Co-Authored-By: Claude Opus 4.7 <[email protected]>
Adds a cppcheck CI job that scans src/ + libretro.c for warning, performance, and portability issues. Excludes vendored / generated trees (src/m68000, src/bios/jag*.c, libretro-common) at scan time. Known pre-existing findings are pinned in cppcheck-suppressions.txt with one-line rationales (false positives where cppcheck doesn't model GET16/GET32, plus a handful of upstream signed-shift idioms flagged for follow-up). --error-exitcode=1 means new findings outside the suppression set fail CI -- keeps the baseline from silently growing. Local baseline (with the suppressions) is 0 findings. Co-Authored-By: Claude Opus 4.7 <[email protected]>
The macos-13 (Intel) runner pool is increasingly scarce -- v2.2.0's release job was queued for over an hour on this single row before we cancelled it and shipped manually. Switch the macos-x86_64 builds (in both release.yml and c-cpp.yml) to run on macos-latest (arm64) and cross-compile via `clang -arch x86_64`. BLITTER_SIMD=sse2 is required because Makefile.common's auto-detect keys on host arch (uname -m), not target. Threaded through a new optional matrix field `make_extra` so the Build step can append per-row flags without bloating the per-row matrix entries. Tested locally on Apple Silicon: the cross-compiled .dylib loads cleanly and passes file/lipo identification as Mach-O x86_64. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Three build/CI bugs surfaced by PR #121 first run: 1. Makefile -- the new `$(CORE_DIR)/libretro.o: $(VERSION_H)` rule sat above `all:`, so Make 3.81 (the stock /usr/bin/make on macOS, also the GitHub Actions macos-latest runner) latched onto libretro.o as the default goal and the build silently stopped after one .o. Moved the dependency hook below `all:`, and switched version.h regeneration from a `: FORCE` rule to a parse-time `_VERSION_GEN := $(shell ...)` (avoids a separate parallelism quirk in old Make). Verified: make -j4 now produces the dylib with 46 retro_* exports as expected. 2. cppcheck-suppressions.txt -- cppcheck 2.13 (Ubuntu 24.04 apt) rejects comment lines in suppressions files with "Failed to add suppression. No id." Newer cppcheck (2.20) accepts them. The workflow now strips comments + blank lines before passing the file to cppcheck, keeping the source-of-truth file with rationale intact. 3. c89-lint.sh -- since libretro.c now `#include "version.h"`, the `-fsyntax-only` lint failed on a fresh checkout where `make` hadn't run yet. c89-lint.sh now self-bootstraps version.h via `scripts/gen-version-h.sh` so it works standalone. Also addresses Copilot review comments on the CLAUDE.md edit: - C89 exempt list now matches the actual scripts/c89-lint.sh::skip_file patterns (cpu*.c / read*.c / jag*bios*.c / SIMD / test tools), not the over-broad `src/m68000/*`. - Memory map reference points at src/core/vjag_memory.c (where the address-range comment lives) instead of vjag_memory.h. - Removed dangling references to non-existent test/headless.py and libretro.py; the actual harness is test/regression_test.sh + miniretro. - Removed the `make screenshots-preflight` reference (no such target). Co-Authored-By: Claude Opus 4.7 <[email protected]>
ndk-build uses jni/Android.mk, not the project Makefile, so the parse-time `_VERSION_GEN := $(shell ...)` doesn't run and libretro.c fails with "version.h: file not found". Same shape as the msvc-check fix: explicitly call scripts/gen-version-h.sh before invoking ndk-build, in both c-cpp.yml and release.yml. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Establishes the GitFlow branching model (develop / release/* / hotfix/*) and wires the CI to support it. master remains the default GitHub branch (so clones / "View raw" / etc. still resolve). Workflow trigger updates: - c-cpp.yml, regression-test.yml: push fires on [master, develop], pull_request on [develop, master]. Means CI runs on PRs to either branch and on every push that lands. - pull-request.yml: auto-PR for feature/* branches now targets develop, not master. New workflow: - warn-pr-base.yml: comments on PRs that target master, asking the contributor to retarget to develop. Skipped when the source branch is release/* or hotfix/* (the legitimate master-bound exits). Uses pull_request_target so it has write perms on fork PRs; only reads PR metadata so this is safe. Idempotent via a marker comment. New docs: - .github/PULL_REQUEST_TEMPLATE.md: prompts contributors to verify base is develop, with an explicit hotfix/release escape hatch. - docs/release-process.md: documents the full flow including back-merge after tagging and a hotfix recipe. - CLAUDE.md: short branching paragraph so future Claude sessions default to branching off develop. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Three small CI hygiene wins: 1. .github/dependabot.yml -- weekly Monday auto-PR for github-actions ecosystem updates against develop. Would have surfaced the Node 20 deprecation as a routine bump-PR weeks before CI started yelling. 2. concurrency: blocks on c-cpp.yml + regression-test.yml. PRs cancel in-progress runs when new commits push (saves CI minutes); pushes to master/develop run to completion (always have a green/red record). 3. scripts/check-info-version.sh + new c89-lint job step -- asserts dist/info/virtualjaguar_libretro.info display_version matches the Makefile CORE_BASE_VERSION on every build. Catches release-prep skew before it reaches users via libretro-super. Co-Authored-By: Claude Opus 4.7 <[email protected]>
CONTRIBUTING.md is the human-facing complement to CLAUDE.md. Covers GitFlow branching, build / test / lint commands, pre-commit setup, commit-message style, and the C89 reminder. scripts/install-hooks.sh now also runs check-info-version.sh in the pre-commit pipeline whenever dist/info/.info or Makefile is staged -- catches a stale display_version bump before it reaches CI. Co-Authored-By: Claude Opus 4.7 <[email protected]>
New \"sanitizers\" job in c-cpp.yml builds with
-fsanitize=address,undefined -O1 -g and runs the full make test
suite with ASAN_OPTIONS / UBSAN_OPTIONS set strict.
Initially marked continue-on-error: true so the first run is
advisory -- the cppcheck job already flagged a couple of signed-shift
UB sites in src/tom/{gpu,op}.c that UBSAN will hit immediately, and
ASAN may surface other latent issues. Triage them, fix or suppress,
then flip to gating.
Local sanity: clang -fsanitize=address,undefined builds the dylib
cleanly on darwin-arm64 (Mach-O x86_64 dlsym path is exercised in CI).
Co-Authored-By: Claude Opus 4.7 <[email protected]>
.clang-format: tolerant config for the existing mixed-indent codebase. UseTab: ForIndentation accepts both tab and 4-space indent without rewriting one to the other. ColumnLimit: 0 keeps line-length advisory (no auto-rewrap). Enforces brace placement, spacing, and pointer alignment that ARE consistent across the tree. clang-format.yml workflow runs clang-format-diff -p1 against the PR diff hunks ONLY (-U0), so existing files are invisible to it -- only new/changed lines are checked against the config. Vendored / generated trees excluded via git pathspec. continue-on-error: true so it surfaces as advisory, not a merge gate, while contributors get used to the convention. Job summary emits the suggested diff for easy copy-paste. Co-Authored-By: Claude Opus 4.7 <[email protected]>
.clang-tidy: curated check list (bugprone-* + clang-analyzer-* + a few readability checks) with disables for the noise that always misfires on emulator C (narrowing-conversions on register byte ops, macro-parentheses on GET16/SET32 byte-swap macros, reserved-identifier on UAE 68K __regs idioms, etc.). clang-tidy job runs only on pull_request events. Generates compile_commands.json via bear, then runs clang-tidy on the PR-changed .c/.h files only (filtered to in-tree code; excludes libretro-common, src/m68000, src/bios/jag*.c, generated version.h). continue-on-error: true so the first run surfaces the baseline without blocking PRs. Plan: triage findings on the first 1-2 PRs, suppress / fix as appropriate, then flip to gating. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Adds: - Makefile: COVERAGE=1 toggle injects --coverage -O0 -g into FLAGS + LDFLAGS. New \`make coverage\` target does clean + COVERAGE=1 TEST_EXPORTS=1 build + make test + gcovr report. - gcovr.cfg: filters to src/, excludes libretro-common, src/m68000, src/bios/jag*.c, test/. Cobertura XML output. - codecov.yml: per-PR patch threshold 0%, project threshold 1% (noise absorption), same ignore globs. - c-cpp.yml: new \"coverage\" job runs make coverage and uploads via codecov-action@v5. Token optional (public repo); set CODECOV_TOKEN secret to skip the anonymous-upload rate limit. continue-on-error: true so coverage runs are advisory. Codecov requires the GitHub App to be installed on the libretro org to post the per-PR comment + status -- one-time admin step. Co-Authored-By: Claude Opus 4.7 <[email protected]>
The harness at test/tools/test_benchmark.c already existed but had no build wiring -- it just sat there waiting. Now \`make benchmark\` builds the harness and runs it against a default ROM (test/roms/yarc.j64), with overrides for ROM, frame count, warmup, and blitter mode: make benchmark # 600 frames, fast blitter make benchmark BENCH_FRAMES=3000 # longer = smoother numbers make benchmark BENCH_BLITTER=accurate # A/B against slow path make benchmark BENCH_ROM=test/roms/private/X.j64 Local sanity: ./test/tools/test_benchmark on yarc.j64 reports ~240 FPS / 4.2ms per frame on Apple Silicon, fast blitter. Use as a same-host commit-to-commit delta during the upcoming perf work. docs/profiling.md is the new reference: macOS Instruments invocation, Linux perf + flame graph, the SIMD A/B knob, known hot paths (OPProcessFixedBitmap, BlitterMidsummer2, GPUExec, DSPExec), build flavors for profiling vs sanitizing vs coverage, and a regression-triage checklist. CONTRIBUTING.md and CLAUDE.md cross-reference the new doc. The harness binary is built inline in the benchmark recipe (not via a separate rule under the TEST_EXPORTS=1 block), so \`make benchmark\` works without TEST_EXPORTS=1 -- the harness only uses the production retro_* exports. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Three GitHub issue forms (yml schema) under .github/ISSUE_TEMPLATE/: - bug_report.yml -- functional bugs. Pre-fills the questions we always end up asking: which game, which BIOS mode, which blitter mode, which core version + frontend, log output. - performance_issue.yml -- separate template for slowdown / stutter reports, with a prompt for `make benchmark` numbers and the blitter-mode A/B (most perf reports are blitter-specific). - feature_request.yml -- libretro-surface feature asks; redirects hardware-emulation feature requests to upstream Virtual Jaguar. config.yml disables blank issues and adds two contact links: the libretro Discord (for general questions that aren't core bugs) and the upstream repo (for issues reproducible in standalone Virtual Jaguar). Co-Authored-By: Claude Opus 4.7 <[email protected]>
Adds .github/labeler.yml + .github/workflows/labeler.yml using actions/labeler@v5. Any PR that touches a subsystem gets the matching label so triage queries work: is:pr label:dsp is:pr label:blitter label:performance is:pr label:tests author:JoeMatt Subsystem mappings: dsp src/jerry/dsp*, test/test_dsp_*, test/test_audio_* gpu src/tom/gpu*, test/test_gpu_* blitter src/tom/blitter*, test/test_blitter_* op src/tom/op*, test/test_op_* m68k src/m68000/**, test/test_m68k_* cd src/cd/**, test/test_cd_* bios src/bios/**, test/test_hle_bios.c core src/core/**, test/test_subsystem_*, test/test_event_*, ... Area mappings reuse existing labels where possible (`ci/cd 🤖`, `📖 documentation`, `performance`) and add `libretro-glue`, `tests`, `build`. sync-labels: true means labels auto-removed if a follow-up commit reverts the relevant files. 11 new labels created on the repo (subsystem + area). Co-Authored-By: Claude Opus 4.7 <[email protected]>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Umbrella PR to establish CI hygiene and code-quality gates, adopt a GitFlow-style branching model (develop integration branch), and add tooling for versioning, symbol-export validation, static analysis, and coverage.
Changes:
- Introduces GitFlow branching guidance + a workflow that warns when PRs target
masterdirectly. - Adds generated
src/core/version.has the single source-of-truth version mechanism and wires it into builds/CI. - Adds/updates CI jobs for EditorConfig, cppcheck, Mach-O symbol export gating, advisory clang-format/clang-tidy/sanitizers, and coverage (gcov + Codecov).
Reviewed changes
Copilot reviewed 38 out of 47 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tom/tom.c | Removes trailing whitespace lines for style hygiene. |
| src/tom/op.c | Fixes trailing whitespace in a comment. |
| src/tom/gpu.c | Removes trailing whitespace. |
| src/tom/blitter.c | Fixes trailing whitespace in a comment. |
| src/jerry/jerry.h | Removes trailing whitespace. |
| src/cd/cdrom.c | Fixes trailing whitespace in an embedded comment block. |
| scripts/install-hooks.sh | Enhances pre-commit hook to run c89-lint and .info-version checks. |
| scripts/gen-version-h.sh | Adds generator for src/core/version.h from Makefile + git rev. |
| scripts/check-info-version.sh | Adds script to validate .info display_version vs Makefile version. |
| scripts/c89-lint.sh | Ensures generated version.h exists before syntax-only linting. |
| libretro.c | Switches CORE_VERSION definition to generated version.h. |
| gcovr.cfg | Adds gcovr configuration for coverage filtering. |
| exports.list | Adds Mach-O production exported symbols list (retro_* only). |
| exports-test.list | Adds Mach-O test exported symbols list for TEST_EXPORTS=1 builds. |
| docs/release-process.md | Documents GitFlow branching and release/hotfix flows. |
| cppcheck-suppressions.txt | Adds documented cppcheck suppressions for CI. |
| codecov.yml | Adds Codecov config for patch/project thresholds and ignore paths. |
| Makefile | Adds CORE_BASE_VERSION SoT, Mach-O export gating, version.h generation, and coverage target. |
| CONTRIBUTING.md | Adds contributor guide including GitFlow, build/test/lint instructions. |
| CLAUDE.md | Refactors and updates repo guidance to match new workflows/tooling. |
| .gitlab-ci.yml | Normalizes line endings/formatting (LF) for consistency. |
| .gitignore | Ignores generated src/core/version.h. |
| .github/workflows/warn-pr-base.yml | New workflow to comment on PRs targeting master (GitFlow). |
| .github/workflows/version-bump.yml | Updates version bump SoT to Makefile CORE_BASE_VERSION + bumps checkout action. |
| .github/workflows/release.yml | Updates actions versions + adds macOS x86_64 cross-build and version.h generation for NDK. |
| .github/workflows/regression-test.yml | Retargets to develop+master, adds concurrency cancel, bumps actions versions. |
| .github/workflows/rebase.yml | Bumps checkout action version. |
| .github/workflows/pull-request.yml | Retargets auto-PR branch to develop (GitFlow). |
| .github/workflows/claude.yml | Bumps checkout action version. |
| .github/workflows/claude-code-review.yml | Removes workflow. |
| .github/workflows/clang-format.yml | Adds advisory clang-format-on-changed-lines workflow. |
| .github/workflows/c-cpp.yml | Retargets CI to develop+master, adds concurrency, cppcheck/editorconfig/coverage/sanitizers/clang-tidy and other gates. |
| .github/workflows/artifacts.yml | Bumps github-script action version. |
| .github/dependabot.yml | Adds Dependabot config targeting develop. |
| .github/PULL_REQUEST_TEMPLATE.md | Adds PR template aligned with GitFlow + test plan checklist. |
| .github/CODEOWNERS | Fixes trailing whitespace. |
| .editorconfig | Adds EditorConfig rules + exemptions for generated/vendored content. |
| .ecrc | Adds editorconfig-checker exclude rules. |
| .clang-tidy | Adds curated clang-tidy configuration. |
| .clang-format | Adds tolerant clang-format configuration for mixed-indentation codebase. |
Comments suppressed due to low confidence (3)
scripts/install-hooks.sh:1
- The generated pre-commit hook assumes it runs from the repo root and passes
$STAGED_Cunquoted, which can break if git runs hooks from a different working directory and/or if filenames contain spaces or other special characters. Consider resolving the repo root inside the hook (e.g., viagit rev-parse --show-toplevel), invoking scripts via absolute paths, and using NUL-delimited file lists (git diff --name-only -z+xargs -0or awhile IFS= read -r -d ''loop) to handle filenames safely.
scripts/gen-version-h.sh:1 - When git metadata isn’t available (e.g., source tarballs or vendored builds), this will bake
unknownintoCORE_VERSION(e.g.,vX.Y.Z unknown). That can be misleading in RetroArch UI and logs. Consider conditionally definingGIT_VERSIONas an empty string whenGIT_REVisunknown, so the version string remains clean while still including the revision when available.
scripts/gen-version-h.sh:1 - When git metadata isn’t available (e.g., source tarballs or vendored builds), this will bake
unknownintoCORE_VERSION(e.g.,vX.Y.Z unknown). That can be misleading in RetroArch UI and logs. Consider conditionally definingGIT_VERSIONas an empty string whenGIT_REVisunknown, so the version string remains clean while still including the revision when available.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The codebase has mixed style per-file (Allman braces in src/jerry/jerry.h, K&R elsewhere; tabs in src/tom/, spaces in libretro.c). No single .clang-format config can enforce a single style without flagging mountains of pre-existing code. Trim the .clang-format config to bare-minimum (don't reflow long lines, don't sort includes, don't touch preprocessor indent), and change the workflow to ALWAYS exit 0 -- suggestions are emitted to the job summary as a "consider this" prompt, never a gate. Future work: a deliberate one-time reformat campaign could pick a single style and apply it tree-wide. Until then, this job is purely advisory. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Three real bugs flagged in Copilot's second review pass: 1. cppcheck workflow used `grep -E '^\s*(#|$)'` to strip suppressions file comments. `\s` is a GNU grep extension, not POSIX ERE. Switched to `[[:space:]]` for portability. 2. Makefile invoked `_VERSION_GEN := $(shell bash gen-version-h.sh)` on every parse, including read-only goals like `make clean`, `make print-FOO`, `make lint`. Now gated via MAKECMDGOALS filter: read-only / metadata goals skip the bash invocation. Real builds (default goal, all, etc.) still run it. Verified locally: make clean / print-* / lint don't spawn the script; make / make all regenerate version.h as expected. 3. clang-format workflow captured the entire PR diff into a shell variable. Switched to mktemp + stream pipe -- handles large diffs without shell variable bloat. Co-Authored-By: Claude Opus 4.7 <[email protected]>
The previous "always exit 0" intent was foiled by GitHub Actions'
default `bash -e {0}` shell: both git diff and clang-format-diff
return 1 when their output is non-empty (diff present / suggestions
made), which `set -e` treats as a fatal error and kills the script
before our trailing `exit 0` runs.
Switch to `shell: bash {0}` (no -e), keep `set -u`, and add `|| true`
on the clang-format-diff invocation belt-and-suspenders. Also handle
"clang-format-diff not found" as a warning rather than hard error
(this is an advisory check; missing tooling shouldn't fail it).
Co-Authored-By: Claude Opus 4.7 <[email protected]>
The compile step had `-fsanitize=address,undefined` but the link step uses \$(LD) (defaults to cc), so the asan/ubsan runtime libs (__asan_memcpy, __ubsan_handle_*) weren't pulled in -- linker errors before the test step could run. Setting LD=\"clang -fsanitize=...\" alongside CC/CXX makes the link step instrument-aware. The job will now actually run the test suite under sanitizers and surface real findings (or pass clean). Co-Authored-By: Claude Opus 4.7 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR introduces CI and repository hygiene improvements for the Virtual Jaguar libretro core, including GitFlow branching, stricter code-quality checks, version single-source-of-truth generation, symbol export gating, and new contributor/performance documentation.
Changes:
- Add/expand CI jobs and policies (develop integration branch, concurrency cancellation, static analysis, coverage, sanitizers, formatting/tidy advisory checks).
- Introduce generated
src/core/version.hfromCORE_BASE_VERSIONin the Makefile and wire version syncing checks. - Add contributor/release/performance docs plus local tooling (
make benchmark, hooks, configs).
Reviewed changes
Copilot reviewed 38 out of 47 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tom/tom.c | Whitespace cleanup (trailing whitespace removal). |
| src/tom/op.c | Whitespace cleanup in comment. |
| src/tom/gpu.c | Whitespace cleanup (remove trailing space). |
| src/tom/blitter.c | Whitespace cleanup in comment. |
| src/jerry/jerry.h | Whitespace cleanup (remove trailing space). |
| src/cd/cdrom.c | Whitespace cleanup in comment block. |
| scripts/install-hooks.sh | Expand pre-commit hook behavior (lint + version check) and document it. |
| scripts/gen-version-h.sh | New script to generate src/core/version.h from Makefile version + git rev. |
| scripts/check-info-version.sh | New script enforcing .info display_version matches Makefile version. |
| scripts/c89-lint.sh | Ensure generated version.h exists before linting. |
| libretro.c | Switch core version string source to generated version.h. |
| gcovr.cfg | Add gcovr configuration and exclusions. |
| exports.list | Add Mach-O exported symbols list for production dylibs. |
| exports-test.list | Add Mach-O exported symbols list for test builds. |
| docs/release-process.md | Document GitFlow-based release/hotfix process and back-merge step. |
| docs/profiling.md | Add profiling and benchmarking guide. |
| cppcheck-suppressions.txt | Add cppcheck suppressions with rationale comments. |
| codecov.yml | Add Codecov thresholds/ignores and PR comment config. |
| Makefile | Add CORE_BASE_VERSION SoT, version header generation, Mach-O export gating, coverage + benchmark targets. |
| CONTRIBUTING.md | Add contributor guide (GitFlow, build/test/lint commands, hooks). |
| CLAUDE.md | Update AI-facing guidance (GitFlow, build/test, profiling, harnesses). |
| .gitlab-ci.yml | Normalize line endings/whitespace (LF). |
| .gitignore | Ignore generated src/core/version.h. |
| .github/workflows/warn-pr-base.yml | Add PR base warning workflow for GitFlow (master-target PRs). |
| .github/workflows/version-bump.yml | Update version bump workflow to edit Makefile CORE_BASE_VERSION. |
| .github/workflows/release.yml | Update actions versions; add macOS x86_64 cross-build; ensure version.h for NDK. |
| .github/workflows/regression-test.yml | Retarget to develop/master, add concurrency cancellation, bump actions versions. |
| .github/workflows/rebase.yml | Bump checkout action version. |
| .github/workflows/pull-request.yml | Retarget PR-creation workflow default base to develop. |
| .github/workflows/labeler.yml | Add PR labeler workflow. |
| .github/workflows/claude.yml | Bump checkout action version. |
| .github/workflows/claude-code-review.yml | Remove workflow. |
| .github/workflows/clang-format.yml | Add advisory clang-format-on-changed-lines workflow. |
| .github/workflows/c-cpp.yml | Retarget CI to develop/master; add concurrency; add editorconfig, coverage, sanitizers, clang-tidy, cppcheck; bump actions versions; add Mach-O leak check. |
| .github/workflows/artifacts.yml | Bump github-script action version. |
| .github/labeler.yml | Add subsystem/area label mapping for PR labeler. |
| .github/dependabot.yml | Add Dependabot config targeting develop. |
| .github/PULL_REQUEST_TEMPLATE.md | Add PR template reflecting GitFlow + test plan checklist. |
| .github/ISSUE_TEMPLATE/performance_issue.yml | Add performance issue template. |
| .github/ISSUE_TEMPLATE/feature_request.yml | Add feature request template. |
| .github/ISSUE_TEMPLATE/config.yml | Add issue template config and contact links. |
| .github/ISSUE_TEMPLATE/bug_report.yml | Add bug report template. |
| .github/CODEOWNERS | Whitespace cleanup (remove trailing space). |
| .editorconfig | Add EditorConfig rules and exclusions for generated/vendored content. |
| .ecrc | Add editorconfig-checker exclusion config. |
| .clang-tidy | Add clang-tidy configuration (curated checks). |
| .clang-format | Add minimal clang-format configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The first clang-tidy run surfaced 30 findings. After triage: REAL BUGS FIXED: - src/tom/op.h: OPProcessList prototype said \`scanline\` but the definition in op.c uses \`halfline\` (correct -- it's called once per video halfline, not per scanline). Renamed the prototype to match. [readability-inconsistent-declaration-parameter-name] - src/cd/cdrom.c:208: redundant \`return;\` at end of a void function. Removed; replaced with comment explaining the no-op-for-now stub. [readability-redundant-control-flow] CHECKS DISABLED with documented rationale (added to .clang-tidy): - bugprone-switch-missing-default-case (8 findings): cosmetic noise on bit-field decode switches that exhaustively cover their value space. - bugprone-branch-clone (10 findings): src/cd/cdrom.c and src/tom/tom.c use intentional register-decode if-chains where several adjacent register addresses produce the same value, on purpose, for legibility. Real clones get caught in code review. - clang-analyzer-deadcode.DeadStores (8 findings): blitter / OP register-decode functions self-doc-init locals that are read via macros (BCOMPEN, DSTA2, ...) clang-tidy can't see the linkage for. Removing those inits would introduce real bugs. - clang-analyzer-optin.portability.UnixAPI (1 finding): false positive on libretro_core_options.h calloc(0) -- vendored. Net result: clang-tidy CI job should now exit clean on this PR's diff. When future PRs introduce real bugs in the kept-enabled checks, those will fire. Co-Authored-By: Claude Opus 4.7 <[email protected]>
When building a shared library with -fsanitize, clang doesn't link the static asan/ubsan runtime into the .so itself; it expects the host executable to provide them. Our SHARED rule passes -Wl,--no-undefined which then rejects the unresolved sanitizer symbols (__asan_memcpy, __ubsan_handle_*). -shared-libasan switches clang to use libclang_rt.asan-*.so, which the host process picks up at dlopen time. Set LD_LIBRARY_PATH for the test binaries so they can find it at runtime. Co-Authored-By: Claude Opus 4.7 <[email protected]>
The first sanitizer run (now correctly linking with -shared-libasan) caught real UB in src/m68000/readcpu.c:1004 -- "shift exponent -1 is negative". The UAE 68K core (machine-generated, ~1.8 MB cpuemu.c) uses negative shifts intentionally; fixing requires patching the generator upstream and is out of scope. We treat src/m68000/ as opaque per CLAUDE.md. Add .ubsan-ignorelist with src:src/m68000/* and pass it via -fsanitize-ignorelist so the sanitizer job isolates new bugs in OUR code without burying the signal under upstream noise. Also fix docs/profiling.md: SIMD is currently wired into the ACCURATE blitter's pixel kernel (BlitterMidsummer2's DATA() macro), not the fast blitter as the doc claimed. Caught by the blitter spike research agent. Cross-references issue #124 for the SIMD widening plan. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Five real findings: 1. scripts/gen-version-h.sh: shebang said #!/usr/bin/env bash but the script is pure POSIX sh. Switched to /usr/bin/env sh so it runs in environments without bash. Updated the header comment to describe the actual call sites (Makefile parse-time $(shell), Android.mk, c89-lint, install-hooks pre-commit, msvc-check). 2. scripts/c89-lint.sh: now invokes the generator via \`sh\`, not \`bash\` -- so the C89 lint hook works on POSIX-only environments. 3. jni/Android.mk: ndk-build doesn't go through the project Makefile, so the parse-time version.h gen there didn't fire for local Android builds (only c-cpp.yml's CI step had it). Added the \`$(shell sh ... gen-version-h.sh ...)\` invocation to Android.mk. Also dropped the now-redundant -DGIT_VERSION wiring (handled by the generated header). 4. scripts/install-hooks.sh header comment: corrected dist/info/.info path reference to "anything under dist/info/ or Makefile". 5. Makefile \`make benchmark\` target: -ldl is Linux-specific and was hard-coded. macOS clang accepts it as a no-op but other linkers may not. Now conditional via \`$(if $(filter Linux,$(uname)),-ldl)\`. Local sanity: sh scripts/gen-version-h.sh runs clean, c89-lint passes, make benchmark on Apple Silicon produces expected ~240 FPS without spurious -ldl warnings. Co-Authored-By: Claude Opus 4.7 <[email protected]>
This workflow was failing on every PR (action exited non-zero after the bun-install step) and generating noise in the Actions tab. Already removed on `develop` via PR #121 commit 10de094 -- this is the direct cleanup on master so the workflow stops triggering until the next develop -> master merge. Co-Authored-By: Claude Opus 4.7 <[email protected]>
fix: ASAN-surfaced bugs (closes #125, #127) Six commits, each independently reviewable, addressing every issue the sanitizers job uncovered after PR #121 wired it up: - table68k + branch_condition_table leaked across dlopen/dlclose cycles (closes #125; symmetric m68k_done / GPUDone wired into JaguarDone, with free_table68k() in readcpu.c so ownership stays inside the module that allocates). - test_hle_bios.c was missing a final p_retro_unload_game() so JaguarDone() never ran for the PAL load -> the leak fix above didn't appear to take effect until that test was corrected. - 1-byte global-buffer-overflow read past tomRam8 in five tom_render_*_scanline() functions (closes #127); new tom_clamp_line_buffer_width() helper used in all five. - Rotate-by-zero UB in 6 ROR sites and rotate-by-32 UB in 3 RORQ sites across src/tom/gpu.c and src/jerry/dsp.c, fixed via the standard portable `(x >> r) | (x << ((-r) & 31))` idiom plus masking `r` to 0x1F when sourced from *_convert_zero[]. clang-tidy curated check list updated: - bugprone-incorrect-roundings (USEC_TO_*_CYCLES macro pattern) - bugprone-multi-level-implicit-pointer-conversion (dlsym idiom) - clang-analyzer-optin.performance.Padding (UAE struct layouts) CI: 30/30 green including the previously-advisory sanitizer job. Candidate to flip continue-on-error: true -> false on that job in a follow-up.
Summary
Umbrella PR establishing CI hygiene, code-quality gates, GitFlow branching, performance tooling, and contributor docs. 20 commits, each independently reviewable. Targets
develop(the new integration branch).Commits
4c321e610de094claude-code-review.yml5e6ca34738e4493ba3ad6src/core/version.hfrom Makefile (single SoT)c9122c6913738947821a7e9b605801400b4version.hforndk-buildfe10eec2b3c4d2.infoversion check50954e89f8e831454e3b494476928e05b86ee93fe5make benchmarktarget +docs/profiling.md3a506d591721e9Highlights
Branching
developintegration branch,masterrelease-only. PRs to master get an auto-redirect comment.Defensive ABI
.dylib/.so/.dllexports only_retro_*on every supported platform. Mach-Oexports.list+link.T/link-test.Tfor ELF. CI verifies no leaks.Single source-of-truth version
CORE_BASE_VERSIONin Makefile is the only place to bump.version.hgenerated,libretro.creads it,version-bump.ymlwrites it,.infovalidated against it on every build.Six strict CI gates
Four advisory CI gates (will tighten after first findings triaged)
Performance tooling
make benchmarkrunstest/tools/test_benchmarkheadless against a fixed ROM (defaultyarc.j64, 600 frames) — same-host commit-to-commit perf delta in <5 sec.docs/profiling.mdcovers Instruments /perf/ flame graphs, theBLITTER_SIMDA/B knob, and a hot-paths cheat sheet.Repo hygiene
dsp,gpu,blitter,op,m68k,cd,bios,core,libretro-glue,tests,build,performance).What's needed from a maintainer / admin (separate from this PR)
libretroorg and enable the repo so the per-PR coverage comment + status check show up. OptionalCODECOV_TOKENsecret skips anonymous-upload rate limits.continue-on-error: true→falseon the sanitizers / clang-tidy / clang-format / coverage jobs once a couple of PRs have triaged the baseline findings.Test plan
make -j4produces dylib with 46_retro_*exports, 0 leaksTEST_EXPORTS=1 make testproduces wide ABI (233 exports), test suite passesbash scripts/c89-lint.shpasses from a fresh checkout (155 files)bash scripts/check-info-version.shpasses (v2.2.0 == v2.2.0)COVERAGE=1 make ...toggles--coverage -O0 -gcorrectlyCORE_BASE_VERSIONtriggers libretro.o rebuild + new string in dylibmake benchmark BENCH_FRAMES=30runs cleanly (~240 FPS / 4.2ms/frame on M2 with yarc.j64)develop-targeted run with all new jobs (in progress)Branch base
develop— this PR is part of establishing GitFlow